Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: show how to use to git to submit smaller and faster PRs #83839

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Jan 11, 2025

The documentation had for a long time a section that specifically recommends to submit "smaller PRs" for review:
https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs

Yet submitters keep submitting large PRs on a regular basis, sometimes very large ones. See a couple of very recent examples below.

(Reminder: submitting a giant, draft PR for pure testing purposes and NOT for review is a perfectly fine)

The "natural" explanation is that submitters optimistically and wrongly believe that dumping a large amount of code at once onto reviewers will be dealt with faster than in smaller chunks. This is most likely a contributing factor but most people should quickly learn from bad experience. Yet we keep seeing large PRs on a regular basis. So there must be other factors too.

Based on personal but fairly extensive git support experience, another top reason is likely git usability and some lack of git knowledge (neither the first nor the last time git usability would have a significant impact)

To help with this, add to the existing git guide the simple command that lets split a large submission in several, smaller PRs. This can only help demystify and promote smaller PRs while barely growing the size of the documentation.

While at it, also add a couple missing benefits of smaller PRs.

Recent examples of large PRs:

Every time any one person requests a rebase that changes the PR,
unless there's consensus, there's no mechanism (manual/project process
or built into GitHub) to know/prevent a different person from rejecting
the new changes.

That PR had 21 commits (18 in the final version), 82 files changed and 400 (!) comments. The sheer size massively increased the likelihood of the problem described. Re-submitting it in smaller chunks would obviously have mitigated that problem. Yet that PR was never split and stayed huge...?

  • In this second example, a large PR was submitted with different authors. A disagreement emerged about squashing across different authors:
    Introduce Bouffalo Lab SoC's #78795 (comment) If this PR had been split in smaller chunks, then the squashing discussion might have been avoided entirely. Whether squashing is good or bad in this particular case is irrelevant (and already discussed at great in length in doc: contribution guidelines: Clarify and extend #83117). What matters here is: the submitter lost that chance by submitting a larger PR with different authors.

@kartben
Copy link
Collaborator

kartben commented Jan 11, 2025

This is great—it really is—and I will provide feedback on the suggested additions that I don't see any reason not to include.
What this doesn't address (not that I have an obvious solution to this) is the double standard that can be, and I'm pretty sure will still be, observed where "experienced" maintainers/contributors can get away just fine with sometimes really large PRs that touch multiple areas (sometimes for good reason, as maybe said PRs are already as small as can be, but certainly not always), making it less than obvious to newcomers why they would be doing anything wrong after all given what they see on other PRs. You took two PRs touching boards/SoCs and this is certainly an area where large PRs from vendor themselves tend to not necessarily be frown upon. In fact—and not saying this is done intentionally—they sometimes pack so much that the somewhat opposite problem arises: PR gets merged easy-peasy, but contains changes that should have actually been submitted separately and would have had a different assignee, except they didn't see the PR in time...

To put it differently, I find it unfair to tell contributors something along the lines of "RTFM wrt how to 'properly' contribute" when their natural tendency will always be to use common sense and what they observe as being some kind of norm. "Do as we say, not as you see...".

Am I making sense? Thoughts?

@marc-hb
Copy link
Collaborator Author

marc-hb commented Jan 11, 2025

I totally get what you mean and I think what you call "double standards" and "unfair", others would call it... "trust"?

Contributors with a good track record tend to get less scrutiny whereas brand new people rarely get any slack. I don't think this is intrinsically wrong because the risk level is actually different and trusting old timers reduces the total review workload which is always, always a problem[*]. But there is a very fine balance and it can definitely go too far sometimes.

[*] code reviews are like everything else in QA: everyone wants quality but no one wants to pay for it.

What this doesn't address (not that I have an obvious solution to this)

Me neither - I don't think there is any easy way to solve this besides maintainers trying to keep an eye on their own biases and helping each other. This is not an exact science.

If anything at all, I hope this PR only helps mitigate such "unfairness" by clarifying a guideline that should apply to all?

BTW there is a common misconception that newcomers cannot review code. It is true that new reviewers cannot be as thorough but they can still be very useful:

  • new eyes can help spot tribal knowledge
  • they can still find issues and lower the total review workload
  • great way to learn and progress
  • last but not least: only way to notice and draw attention to such "double standards"!

where large PRs from vendor themselves tend to not necessarily be frown upon.

This is a bit of a special case because... who cares if vendors break ONLY their own products? Small exaggeration to get the point across, it's getting late. You see what I mean.

but contains changes that should have actually been submitted separately and would have had a different assignee, except they didn't see the PR in time...

Now that is 100% wrong and should be flagged - and probably reverted temporarily!

making it less than obvious to newcomers why they would be doing anything wrong after all given what they see on other PRs.

What I'm trying to reinforce in this PR is: smaller PRs are also in the interest of the submitter themselves. Even experienced people who managed to fly quickly "under the radar" don't want to end up wasting time post merge dealing with regressions, drama and reverts. Do they?

@marc-hb marc-hb marked this pull request as draft January 11, 2025 21:04
andyross
andyross previously approved these changes Jan 12, 2025
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems useful. My immediate thought goes to all the giant PRs I've pushed over the years, but in general subsystem-/tree-wide changes aren't really the subject of general docs. For sure we can all agree this is the kind of PR we'd prefer to see reviewed and merged.

@marc-hb marc-hb force-pushed the stacked-diffs branch 2 times, most recently from 36dce5b to 0c1fe17 Compare January 12, 2025 19:19
The documentation had for a long time a section that specifically
recommends to submit "smaller PRs" for review:
https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs

Yet submitters keep submitting large PRs on a regular basis, sometimes
very large ones. See a couple of very recent examples below.

(Reminder: submitting a giant, draft PR for pure _testing_ purposes and
NOT for review is a perfectly fine)

The "natural" explanation is that submitters optimistically and wrongly
believe that dumping a large amount of code at once onto reviewers will
be dealt with faster than in smaller chunks. This is most likely a
contributing factor but most people should quickly learn from bad
experience. Yet we keep seeing large PRs on a regular basis. So there
must be other factors too.

Based on personal but fairly extensive git support experience, another
top reason is likely git usability and some lack of git
knowledge (neither the first nor the last time git usability would have
a significant impact)

To help with this, add to the existing git guide the simple command that
lets split a large submission in several, smaller PRs. This can only
help demystify and promote smaller PRs while barely growing the size of
the documentation.

While at it, also add a couple missing benefits of smaller PRs.

Recent examples of large PRs:

- In the controversial and giant PR
zephyrproject-rtos#77368 (comment)
the exhausted submitter wrote:

> Every time any one person requests a rebase that changes the PR,
> unless there's consensus, there's no mechanism (manual/project process
> or built into GitHub) to know/prevent a different person from rejecting
> the new changes.

That PR had 21 commits (18 in the final version), 82 files changed and
400 (!) comments. The sheer size massively increased the likelihood of
the problem described.  Re-submitting it in smaller chunks would
obviously have mitigated that problem. Yet that PR was never split and
stayed huge...?

- In this second example, a large PR was submitted with different
authors. A disagreement emerged about squashing across different
authors:
zephyrproject-rtos#78795 (comment)
If this PR had been split in smaller chunks, then the squashing
discussion might have been avoided entirely. Whether squashing is good or
bad in this particular case is irrelevant (and already discussed at
great in length in zephyrproject-rtos#83117). What matters here is: the submitter lost
that chance by submitting a larger PR with different authors.

Signed-off-by: Marc Herbert <[email protected]>
@marc-hb marc-hb marked this pull request as ready for review January 12, 2025 20:17
@marc-hb
Copy link
Collaborator Author

marc-hb commented Jan 12, 2025

v2: added new item: "Better test coverage AFTER merge".
Re-ordered items a tiny bit.

@andyross 's approval lost sorry (I know it's a pet peeve of yours...)

@keith-zephyr
Copy link
Contributor

What this doesn't address (not that I have an obvious solution to this) is the double standard that can be, and I'm pretty sure will still be, observed where "experienced" maintainers/contributors can get away just fine with sometimes really large PRs that touch multiple areas

I'd argue that we shouldn't be allowing really large PRs by experienced/trusted contributors either. Everything enumerated @marc-hb about small PRs still applies regardless of the contributor. As a community, we should strive to be more consistent in enforcement of policies and recommended practices.

Copy link
Member

@nashif nashif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how verbose do we need to get? This just repeats what was already in the docs:

Over the last year, with 10k PRs, the mediam commit number in PRs was one. I do not thing this is a general problem we have, and those PRs that have multiple commits, probably they have this for a good reason.

Some of the points here probably can go into one of the existing spots I quickly found, I do not think we need to expand on this across the tree repeating ourselves and duplicating the message (which is a common theme in our docs).

and getting code merged before the whole work is complete. This faster
approach is known as "Stacked Diffs".

- Less wasted work if reviewers or maintainers reject the direction of the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is this considered a "benefit"? If the direction is rejected, and part of it were already merged, that is not considered a good thing

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is not new, I merely moved it. I could remove it.

Copy link
Collaborator Author

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nashif for the feedback.

how verbose do we need to get? This just repeats what was already in the docs:
https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs
https://docs.zephyrproject.org/latest/contribute/guidelines.html#contribution-workflow

These are the two pages touched by this PR so you got me confused... I really tried to add only new points but maybe I could combine some and make things shorter. Can you please be more specific and point at what looks redundant?

I'm 99% sure the "Stacked Diffs" git command is new - and also new to a lot of git users in my "git support" experience. It's IMHO the most important addition in this PR and why it's the title.

Some of the points here probably can go into one of the existing spots I quickly found, ...

Did you mean the two references above or something else?

Over the last year, with 10k PRs, the mediam commit number in PRs was one.

I do not thing this is a general problem we have, and those PRs that have multiple commits, probably they have this for a good reason.

The "Smaller PRs" section does not set any hard or even soft limit on the number of commits per PRs (that would be one "authoritative" way to be much shorter!), I think it barely mentions commits at all. Instead of metrics, it lists the "non-exact science" benefits of smaller PRs and CI so submitters and reviewers are better armed to go and find the sweet spot in each particular case.

That section does in practice focuses on "pathological" cases like brand new features and major refactors because they're the ones costing a disproportionate amount of time (for both reviewers and submitters). That's common for many guidelines: in general you don't need them.

I do not think we need to expand on this across the tree repeating ourselves and duplicating the message (which is a common theme in our docs).

I think I hate copy/paste/diverge more than anyone else but there's often a way to be less verbose and I'd love specific suggestions.

https://quoteinvestigator.com/2012/04/28/shorter-letter/

and getting code merged before the whole work is complete. This faster
approach is known as "Stacked Diffs".

- Less wasted work if reviewers or maintainers reject the direction of the
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is not new, I merely moved it. I could remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants